fix: validate taskRef.apiVersion format for custom tasks #9045
Conversation
tekton-robot
added
the
release-note-none
tekton-robot
added
the
size/L
|
The following is the coverage report on the affected files.
|
|
/retest |
1 similar comment
|
/retest |
waveywaves
force-pushed
the
fix-invalid-taskref-apiversion-validation
branch
from
3797773 to
e40bda6
Compare
waveywaves
force-pushed
the
fix-invalid-taskref-apiversion-validation
branch
from
e40bda6 to
cbab5ca
Compare
aThorp96
left a comment
aThorp96
left a comment
There was a problem hiding this comment.
One suggested change, two general questions
Valid formats are "group/version" where group contains at least one dot
Is this strictly true? I know addon api groups conventionally use a dot, but as far as I can tell k8s doesn't pose that as a restriction.
| // isValidAPIVersion validates the format of an apiVersion string. | ||
| // Valid formats are "group/version" where group contains at least one dot. | ||
| // For custom tasks, apiVersion must always be in the "group/version" format. | ||
| func isValidAPIVersion(apiVersion string) bool { |
There was a problem hiding this comment.
Should this also validate that the apiVersion and Kind are registered in the cluster or something? A CustomRun will still be created when the apiVersion and kind dosn't exist in the cluster, however the CustomRun is simply never updated with any status so the pipelinerun times out. E.g. in the below screenshots I modified one of the examples to use a bogus apiVersion which was syntactically correct.
I suppose being registered in the cluster is not sufficient since I've seen people mistakenly use "apiVersion": "tekton.dev/v1", "kind": "Task" in a TaskRef. Maybe that level of validation should be somewhere else? WDYT?
There was a problem hiding this comment.
your're right kube doesn't strictly require dots in API group names. I've removed that restriction. The validation now only checks for group/version format with non-empty group and version parts. On validating whether the apiVersion/kind is registered in the cluster would be useful, but is a separate concern I beleive, it would require runtime cluster access during admission, which is a bigger change. This PR focuses on catching syntactically invalid apiVersions at the webhook validation level. We could track the runtime check as a follow-up issue ? what do you think ?
Add validation to ensure taskRef.apiVersion follows the correct format (group/version) when specified. Previously, invalid apiVersion values like 'invalid-api-version' were accepted, causing PipelineRuns to create unhandled CustomRuns that would timeout. Now Pipeline creation fails immediately with a clear error message when an invalid apiVersion is provided. Fixes tektoncd#9044
waveywaves
force-pushed
the
fix-invalid-taskref-apiversion-validation
branch
from
cbab5ca to
d56067e
Compare
|
/approve |
|
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: vdemeester The full list of commands accepted by this bot can be found here. The pull request process is described here DetailsNeeds approval from an approver in each of these files:
Approvers can indicate their approval by writing |
tekton-robot
added
the
approved
|
/lgtm |
4 participants
Changes
add validation to ensure taskRef.apiVersion follows the correct format (group/version) when specified. Previously, invalid apiVersion values like 'invalid-api-version' were accepted, causing PipelineRuns to create unhandled CustomRuns that would timeout. Now Pipeline creation fails immediately with a clear error message when an invalid apiVersion is provided.
Fixes #9044
Submitter Checklist
As the author of this PR, please check off the items in this checklist:
/kind <type>. Valid types are bug, cleanup, design, documentation, feature, flake, misc, question, tepRelease Notes